Side note: This is global now, so we could just remove this line and/or split this change into a separate PR.

i left it in just in case these changes get backported to other release lines

Ignorable nit: as this is a file, not a REPL, maybe add assert() or console.log() here?

Nit: it seems a bit unexpected to name file content body (no body in fs doc), it seems more natural for http(s) docs. However, I can miss some conventions)

does src/source work?

Yes, it seems more neutral to me.

This is quite tricky to follow, and if there are changes in v8 perhaps tricky to maintain.

Surely toString.call(fn).endsWith('{ [native code ] }') would give just as strong a guarantee?

if there are changes in v8 perhaps tricky to maintain.

just the opposite (ignoring that Function.prototype.toString is standardised now) different engines or different versions of the same engine may represent native functions differently

@guybedford For a reference implementation see this and this. The idea is to use a builtin native method as a template for detection of others. @devsnek has a todo to implement a v8 helper for this too.

Sure, perhaps a comment would help maintenance though - it's pretty tough to follow.

I'm a fan of comments for sure 📖

name.startsWith('bound ').

any particular reason? regex is still faster at this time

👆 It's micro-opt territory here so it's up to personal preference is all.

Interesting, I didn't realise that had changed in v8. Just paying attention to the getter hot path here.

Keep in mind this a few steps/checks into the native check so many values will have already exited with false by then. This check is also performed once per changed value (so not per repeated get request).

This very much does seem on the hot path to me? get -> wrap -> isNative is on every get in the proxy, so this path seems like it will hit every access to bound functions on exports?

This startsWith check is done after the first few check so will only apply to the first access of bound functions and native functions. I don't think there's a need to get hung up on micro-optimizations at this point. If you find the need later then sure we can tweak various ways to detect xyz.

property in this.reflect.exports might be a nicer check here.

is reflect.exports a null object?

hasOwnProperty could work too, just nice to avoid linear lookup if we can.

exportKeys could be made a Set as well

When the receiver is not equal to the target, surely we don't want to be running the update method?

For example, if someone does Object.create(module.exports).asdf = 'asdf', we don't want to be updating the ES module object.

not quite sure i understand this one, but i do know that Object.create(module.exports).asdf = 'asdf' doesn't change anything on module.exports

Exactly, so it shouldn't call the update function, whereas right now it does I believe.

@guybedford

Exactly, so it shouldn't call the update function, whereas right now it does I believe.

I cannot reproduce your concern.

import { isArray } from 'util'
> Object.create(util).isArray  = 'asdf'
'asdf'
> isArray
[Function: isArray]
> util.isArray
[Function: isArray]

Standard set will create a new "own" property on the receiver if it isn't the same as the target and won't update the target normally. Easiest example of this is with Object.prototype not being updated in :

x={}; // has Object.prototype ; ~= Object.create(Object.prototype)
x.toString = null;
assert.notEqual(x.toString, Object.prototype.toString);

This seems to be a slight bug.

@bmeck

Looks like userland can do some stuff since we allow defineProperty trapping that exhibits these behaviors,

In your example above you have removed the own property of fs when you assigned it fs.readFile=null because in the setter it performed delete fs.readFile. So when you logged both fs.readFile values they had a fs.hasOwnProperty("readFile") of false and are logging the Object.prototype.readFile value. The deleteProperty trap should have updated the ns values to be undefined though, so I should check that out.

In my own implementation I get the expected results:

fs 123 from namespace undefined
fs 456 from namespace undefined

@bmeck

In my local tests for the worstTargetEver things are looking alright.

1. we need to add a test to ensure we don't get value since that is an own property on the receiver so we can prevent regressions.

This looks good

import { foo, bar } from "worstTargetEver"
import w from "worstTargetEver"

w.foo = 3
foo // undefined
w.foo // undefined
bar // 3
w.bar // 3

2. right now it could update to Object.prototype.foo since there isn't a descriptor anymore but we still do a non-guarded Get after the Set. I didn't see any existing things in core that would be problematic.

The worstTargetEver example did not result in a value getting assigned to Object.prototype.foo

 Reflect.has(Object.prototype, "foo") // false

This could be because this line of worstTargetEver...

 Object.defineProperty(this, 'foo', {value}) // not on target, need to ensure it isn't picked up

...the this, while not the target, is the proxy, so it's ok.

3. this somewhat shows a problem if setters are defining other properties that might need to be updated.

The example above works as expected, to me at least.

I'll work with @devsnek to add tests and ensure the PR results align with my local.

@bmeck I had the same concern at the beginning, but because of the limited scope of this PR (only applies to core modules) I didn't think we have to be that watertight, because we don't really have those weird setters/getters.

@jdalton which example? we have multiple in this thread.

@TimothyGu since userland is able to create those situations by mutating core I think we still need to at least document how things work. I don't find the synchronization mismatch intuitive, but am not trying to stop this PR as long as it is well understood.

@bmeck @TimothyGu As mentioned above (here and here) there should be no synchronization mismatch or unexpected gotchas once I get a chance to update this PR (I'll add tests too).

Can you provide a few examples for what this is mitigating? Would help a lot to follow this, and a comment would likely help as well.

👆 There should be unit tests for it (or added if not) but the gist of the issue is that native methods (so not plain or bound ones) may be picky about their this binding and need wrapping for general use when separated from their object.

process.cwd.call(new Proxy(process, {}))
// TypeError: Illegal invocation

We should totally file a v8 bug on this though since this doesn't affect NodeChakra.
Once fixed the workaround isn't needed.

Update: V8 bug here.

Amazing, that helps a lot to know! If it seems like it may well be removed by the time this is unflagged, then we really don't need to worry about the perf on this path for now.

Could be worth noting the justification and v8 issue in a comment.

I'm 👍 on adding comments. I'll add moar to my implementation too.

Update:

Once the V8 bug is fixed in addition to dropping the proxy wrapper for native methods we can also conditionally exclude the get trap on the module.exports proxy for exports that are functions or those we expect to be [object Object] (so likely all builtin modules). Since then, the only thing the get trap will be used for is correcting toStringTag values.

Very nice!

Perhaps do the function check here so we don't need to go into the wrap function unnecessarily as this is our hot path.

I wonder which is faster - target[prop] or ReflectGet(target, prop)? Worth thinking of this one from a perf perspective.

doesn't matter, because i have to pass the receiver through

Again, I'm not sure we want to support prototypal getters here? Could they not be a shortpath:

get (target, prop, receiver) {
  if (target !== receiver)
    return ReflectGet(target, prop, receiver);
  // ... target receiver case
}

The get trap here is just to detect a native method that needs wrapping and handle the toString proxy juggle. There is no other editorial bits going on here.

While it's good to be safe, I'm not sure this is a necessary protection here?

👆 It's a precaution since trap handlers, like descriptors, will read inherited traps/descriptors as well as own.
Does Node have a convention established for this?

places needing to avoid Object.prototype use Object.create in the existing codebase but loader has some usage of __proto__ in literals. No linting or style guide rules exist for Node to my knowledge on this topic.

const module = NativeModule.require(id) should work here and avoid a double lookup :)

getCached returns the entire module object, require just gives the exports object

Ah right, I see, we can reconsider the interface if necessary then for future work.

Standard set will create a new "own" property on the receiver if it isn't the same as the target and won't update the target normally. Easiest example of this is with Object.prototype not being updated in :

x={}; // has Object.prototype ; ~= Object.create(Object.prototype)
x.toString = null;
assert.notEqual(x.toString, Object.prototype.toString);

This seems to be a slight bug.

this line gets hit when you do:

import events from 'events';
Object.defineProperty(events, 'defaultMaxListeners', {value: 333})

/*
TypeError: 'defineProperty' on proxy: trap returned falsish for property 'defaultMaxListeners'
    at Function.defineProperty (<anonymous>)
    at file:///Users/bfarias/Documents/node/t.mjs:2:8
    at ModuleJob.run (internal/modules/esm/module_job.js:106:14)
*/

But it should probably report this as an error for redefining like it does right now:

> Object.defineProperty(events, 'defaultMaxListeners', {value: 333})
TypeError: Cannot redefine property: defaultMaxListeners
    at Function.defineProperty (<anonymous>)

@devsnek @bmeck
By changing the defineProperty trap from

if (ReflectDefineProperty(target, prop, descriptor)) {
  update(prop, ReflectGet(target, prop));
  return true;
}
return false;

to

ObjectDefineProperty(target, prop, descriptor);
update(prop, ReflectGet(target, prop));
return true;

we get the expected error:

TypeError: Cannot redefine property: defaultMaxListeners
    at Function.defineProperty (<anonymous>)

Use Reflect.apply instead.

Use finally block instead. Just curious, why is it necessary to reset the property even in case of thrown exception?

I wasn't sure how the runner operates. If it loads tests and runs them in sequence in a shared process then even if there are failed tests augmented environments could effect other tests if not properly reset/cleaned.

@jdalton Each test is in a separate process. The Python test runner spawns node processes in parallel.

Nice!